594899 - Memory leak repeatedly adding and deleting object properties

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

(copied from bug 592556 comment 34)

If an object doesn't have a PropertyTable yet, and deletes happen, slots leak.

  var obj = {a: 0, b: 1, c: 2};
  for (var i = 0; i < 20; i++) {
      delete obj.b;
      obj.b = 3;
      delete obj.c;
      obj.c = 4;
  }
  dumpObject(obj);  // shows many unused slots

This can go on indefinitely. Creating a table doesn't recover the leaked slots.

(If we care enough to fix this, it would be nice to add an assertion somewhere
that would trip if we waste this many slots, then add this code to
js/src/tests.)

Comment 1

[Brendan Eich [:brendan]]

Have a patch in progress here, it's easy. I also smell trouble if we don't fix this, but can't prove it.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

Chatted with brendan.

Currently slotSpan is documented like this:

    uint32 slotSpan;    /* one more than maximum live slot number */

That is incorrect; what it means is

    uint32 slotSpan;    /* some number > the maximum live slot number */

If we fix all leaks (such as this bug), then we would have a much nicer, tighter invariant:

    /*
     * Number of slots in use. All slots from 0 to slotSpan - 1 are
     * either reserved by the class, allocated to properties, or
     * on this object's freelist (see js::PropertyTable::freelist).
     */
    uint32 slotSpan;

This is easier to reason about, so I'm in favor of fixing all the leaks.

Comment 3

[Brendan Eich [:brendan]]

I intend to fix this bug. :-)

Question is how soon. Sayrer, can I have a betaN+?

/be

Comment 4

[Jason Orendorff [:jorendorff]]

There are error paths in JSObject::getChildProperty that return NULL after having successfully called allocSlot(). The slot is leaked in those cases.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> There are error paths in JSObject::getChildProperty that return NULL after
> having successfully called allocSlot(). The slot is leaked in those cases.

I would like to fix this one in the patch for bug 593129. Doing it here makes a mess in getChildProperty that goes away in that bug.

/be

Comment 6

[Till Schneidereit [:till]]

The testcase from comment 0 doesn't create any empty slots anymore, so I'm assuming this is fixed.